Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor pallet recovery to fungibles #1807

Closed
wants to merge 17 commits into from

Conversation

juangirini
Copy link
Contributor

@juangirini juangirini commented Oct 6, 2023

Partial for #226

The Currency (and ReservableCurrency) traits are deprecated after this paritytech/substrate#12951 in favour of the fungible traits.

A few things have changed:

ReservableCurrency::reserve becomes fungible::MutateHold::hold; ReservableCurrency::unreserve becomes fungible::MutateHold::release and ReservableCurrency::repatriate_reserved becomes fungible::MutateHold::transfer_on_hold.
Holds are roughly analogous to reserves, but are now explicitly designed to be infallibly slashed. They do not contribute to the ED but do require a provider reference, removing any possibility of account reference counting from being problematic for a slash. They are also always named, ensuring different holds do not accidentally slash each other's balances.

Also they require an identifier which is configurable and is expected to be an enum aggregated across all pallet instances of the runtime.

In the Pallet Recovery context we only have HoldReason::RecoveryProcessDeposit and HoldReason::ConfigurationDeposit

/// The reasons for the pallet recovery placing holds on funds.
#[pallet::composite_enum]
pub enum HoldReason {
	/// The Pallet holds it as the initial Configuration Deposit, when the Recovery
	/// Configuration is created.
	ConfigurationDeposit,
	/// The Pallet holds it as the Deposit for starting a Recovery Process.
	RecoveryProcessDeposit,
}

Upgrade notes

The pallet_recovery::Config trait now requires new type RuntimeHoldReason:

impl pallet_recovery::Config for Runtime {
	...
	type RuntimeHoldReason = RuntimeHoldReason;
	...
}

Also, construct_runtime needs pallet_recovery::HoldReason:

construct_runtime!(
	pub enum Runtime
	{
		...
		Recovery: recovery::{Pallet, Call, Storage, Event<T>, HoldReason},
		...
	}
)

TODO

  • migration

Comment on lines +243 to +245
#[cfg(feature = "runtime-benchmarks")]
type Currency: Mutate<Self::AccountId>
+ MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set_balance in benchmarking, so Mutate is needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to set custom where bounds for benchmarks, like this.
So it should be possible to add a <T as Config>::Currency: Mutate there.

@juangirini juangirini added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 6, 2023
@juangirini juangirini changed the title [WIP] Refactor pallet recovery to fungibles Refactor pallet recovery to fungibles Oct 6, 2023
@juangirini juangirini marked this pull request as ready for review October 6, 2023 15:27
@juangirini juangirini requested review from a team October 6, 2023 15:27
@juangirini
Copy link
Contributor Author

bot bench $ pallet dev pallet_recovery

@command-bot
Copy link

command-bot bot commented Oct 6, 2023

@juangirini Positional arguments are not supported anymore. I guess you meant bot bench substrate-pallet --pallet=pallet_recovery, but I could be wrong.
Read docs to find out how to run your command.

@juangirini
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_recovery

@command-bot
Copy link

command-bot bot commented Oct 6, 2023

@juangirini https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3893936 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-e46a081d-e4e1-4e66-9848-2f97ca4ca391 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 6, 2023

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3893936 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3893936/artifacts/download.

@georgepisaltu
Copy link
Contributor

Hey, I was already working on moving pallet-recovery to the new fungible interface, but the edit on #226 seems to have been removed. Since you opened the PR first, I'll just stick to reviewing yours then.

@juangirini
Copy link
Contributor Author

I did update #226 before starting this, not sure how that got lost in between. I have just updated it again, sorry to hear that you already spent time on it.

@juangirini
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_recovery

@command-bot
Copy link

command-bot bot commented Oct 11, 2023

@juangirini https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3932248 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 24-e88fb00c-ae67-4a75-bbcb-ca96ca39d01b to cancel this command or bot cancel to cancel all commands in this pull request.

Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing a migration for those runtimes that use this pallet

@@ -187,7 +192,7 @@ type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup
pub struct ActiveRecovery<BlockNumber, Balance, Friends> {
/// The block number when the recovery process started.
created: BlockNumber,
/// The amount held in reserve of the `depositor`,
/// The amount held the `depositor`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The amount held the `depositor`,
/// The amount held by the `depositor`,

…=dev --target_dir=substrate --pallet=pallet_recovery
@command-bot
Copy link

command-bot bot commented Oct 11, 2023

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3932248 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3932248/artifacts/download.

@juangirini
Copy link
Contributor Author

juangirini commented Oct 12, 2023

You're missing a migration for those runtimes that use this pallet

I guess we can't really migrate until we have the MBM PR (#1781) ready. And lazy migrations don't seem to be a great solution as with it
we are unable to remove the Currency trait, and it adds a lot of noise code that will need to be removed eventually

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Comment on lines +243 to +245
#[cfg(feature = "runtime-benchmarks")]
type Currency: Mutate<Self::AccountId>
+ MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to set custom where bounds for benchmarks, like this.
So it should be possible to add a <T as Config>::Currency: Mutate there.

// Acts like a slashing mechanism for those who try to maliciously recover accounts.
let res = T::Currency::repatriate_reserved(
let res = T::Currency::transfer_on_hold(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking about how to do a lazy migration here. Maybe you can require the Currency config trait to be Currency + MutateHold so that it requires currency and fungibles.

Then in this function you can first try to use the new transfer_on_hold function, but if that does not work then fallback to the old repatriate_reserved. Do you think this could work?
Otherwise the MBMs are still in review and will need audit, so its a bit out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of lazy migrations here, cause that will generate some "garbage" code that will live there forever. It seems more like a patch to me than a real solution. It's been a while already since we wanted the traits to be migrated that I would rather wait for the MBM to be ready and apply a proper solution

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4807116

@bkchr
Copy link
Member

bkchr commented Jul 24, 2024

Stale

@bkchr bkchr closed this Jul 24, 2024
@bkchr bkchr deleted the jg/refactor-pallet-recovery-to-fungibles branch July 24, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants